Eliminate global mutable state, move CLI logic to pkg/#423
Draft
Eliminate global mutable state, move CLI logic to pkg/#423
Conversation
ca13545 to
1fd5c47
Compare
The cmd/kaf package had ~65 package-level mutable variables and 7
os.Exit calls outside of main(). This made the CLI completely
untestable and impossible to reason about.
Restructure following rpk's pattern:
- cmd/kaf/main.go is now a 19-line wrapper that calls pkg/cmd.Execute()
- pkg/app/ holds the App struct owning all shared mutable state
- pkg/cmd/{consume,produce,topic,group,node,query,config,completion}/
each export NewCommand(a *app.App) returning *cobra.Command
- pkg/cmd/root.go wires everything together
All command-specific flags are local variables inside factory functions,
not struct fields. os.Exit only appears in main(). The old os.Exit in
readLines/readFull is replaced with error channels, the group commit
goroutines use errgroup, and config select-cluster returns nil instead
of os.Exit(0).
Also extracts kafka operations into proper pkg/ packages:
- pkg/client: franz-go client construction with SASL/TLS/OAuth
- pkg/topic, pkg/group, pkg/node: admin operations
- pkg/avro, pkg/proto, pkg/config: existing packages cleaned up
Deletes dead code: pkg/streams, pkg/partitioner/jvm.go,
cmd/kaf/oauth.go, cmd/kaf/scram_client.go (functionality moved to
pkg/client/auth.go).
1fd5c47 to
b5c749b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Gut the monolithic cmd/kaf package. Replace ~65 package-level mutable variables and 7 os.Exit calls with a proper architecture that is actually testable. Fix 20+ bugs found during code review.
Why
The old code was a single
package mainblob where every command shared mutable globals through package-level vars and init() functions. This made unit testing impossible, parallel test execution impossible, and the whole thing painful to maintain. The only os.Exit should be in main().Implementation details
Follows the rpk CLI pattern:
cmd/kaf/main.gois now 19 lines -- callspkg/cmd.Execute()and that is itpkg/app.Appstruct owns all shared mutable state (I/O, config, decode caches, display flags)pkg/cmd/(consume, produce, topic, group, node, query, config, completion)NewCommand(a *app.App) *cobra.Command-- command-specific flags are local variables, not struct fieldspkg/cmd/root.gowires the command tree and sets upPersistentPreRunEfor config initKey changes to eliminate os.Exit:
readLines/readFullin produce use error channels instead of os.Exiterrgroupinstead of WaitGroup + os.Exitselect-clusterreturns nil instead of os.Exit(0) on user cancelAlso extracts kafka admin operations into proper
pkg/packages (pkg/client,pkg/topic,pkg/group,pkg/node) and deletes dead code (pkg/streams,pkg/partitioner/jvm.go, SASL/OAuth code consolidated intopkg/client/auth.go).Backward compatibility
All commands, subcommands, flags, short flags, and defaults are preserved from the old code. Verified by full audit against master. The
--verboseflag is retained as a deprecated no-op (sarama was removed). Two intentional default changes:group commit --partitiondefault changed from0to-1: old default silently committed only partition 0 when no partition was specified, which was a bug.topic deletenow prompts for confirmation (with--noconfirmto skip).Bug fixes
Crashes:
--key-proto-typeused without--proto-typeData integrity:
Consume:
--limit-messagesnow stops printing exactly at the limit (was printing extra records per poll batch)--limit-messagesis now a total count instead of per-partition (prevents hang on sparse partitions)--tailnow respects--partitionsflag (was always consuming all partitions)--offsetis used with--group(offset is ignored in group mode)Produce:
Topic:
--partition-assignmentsnow works in combination with-p(was dead code in else branch)compact,deletein cleanup.policy, not justcompactkmsg.ConfigSourceDefaultConfigconstant instead of magic number 5Group:
--topicis now required forgroup commit(was silently committing against empty topic)delete-offsetssubcommand (rewritten for franz-go usingkadm.DeleteOffsets)group lsinstead of including broken entriesClient:
Avro:
Other:
Config.ActiveClusterActiveCluster()call inSetCurrentClusteraws_configimport alias toawsconfig(Go convention)ReadOnlyfield from ConfigEntrygo mod tidyReferences
Architectural pattern: rpk CLI structure